Add billing_report_view_v3 with Aviti and Illumina UNION#56
Add billing_report_view_v3 with Aviti and Illumina UNION#56dn10-sanger wants to merge 2 commits into
Conversation
KatyTaylor
left a comment
KatyTaylor
left a comment
There was a problem hiding this comment.
Hey, well done with this large query! It's neatly presented. I can see the Illumina part is unchanged, and you're getting the same list of fields in both main SELECTs, to make the UNION work.
I've found a few potential changes to ask for.
I haven't looked at the Aviti SELECT clause yet, or the ORDER BY, so will come back to that.
| JOIN [warehouse].iseq_run_status_dict d | ||
| ON d.id_run_status_dict = rs.id_run_status_dict | ||
| WHERE d.description = 'qc complete' | ||
| AND DATE(rs.date) >= DATE_SUB(NOW(), INTERVAL 2 YEAR) |
There was a problem hiding this comment.
There is no equivalent WHERE clause on the Aviti query - either for qc_complete status, or date range.
| r.id_eseq_run_tmp, | ||
| rl.lane, | ||
| f.cost_code, | ||
| st.id_study_lims, |
There was a problem hiding this comment.
If you're grouping by study, you should use id_study_tmp, as that is the unique key. id_study_lims is only considered unique when in combination with the id_lims field on study table.
| JOIN [warehouse].eseq_run_lane_metrics rl ON r.folder_name = rl.run_folder_name | ||
| JOIN [warehouse].eseq_product_metrics pm ON rl.id_run = pm.id_run | ||
| JOIN [warehouse].eseq_flowcell f ON pm.id_eseq_flowcell_tmp = f.id_eseq_flowcell_tmp | ||
| LEFT JOIN [warehouse].study st ON f.id_study_tmp = st.id_study_tmp |
There was a problem hiding this comment.
Study is LEFT JOIN for Aviti but JOIN (inner) for Illumina. They should be consistent. I suggest LEFT JOIN so we wouldn't lose data in the case where we somehow manage to get a flowcell without a Study in future.
| f.pipeline_id_lims, | ||
| f.custom_primer_kit_used, | ||
| f.quant_method_used, | ||
| r.run_parameters |
There was a problem hiding this comment.
There are several fields in the GROUP BY that are redundant. For instance, we don't need to group by r.run_parameters because we've already grouped by r.id_eseq_run_tmp, and it's 1:1 with that.
I learnt something new today - if there are fields like this, it's MySQL best practice (according to CoPilot) to leave them out of the GROUP BY, to keep it uncluttered and clear which fields actually change the number of rows returned. For the redundant fields, MySQL will want you to add an aggregator - apparently ANY_VALUE is the one to use - unlike MIN(), it signals to the reader that you know all values in the group are the same, so the choice of which one to return is arbitrary.
| JSON_UNQUOTE(JSON_EXTRACT(r.run_parameters, '$.ThroughputSelection')) AS throughput_selection, | ||
| JSON_UNQUOTE(JSON_EXTRACT(r.run_parameters, '$.KitConfiguration')) AS kit_configuration, | ||
| f.custom_primer_kit_used AS element_customer_primer_kit, | ||
| f.quant_method_used AS element_quant_method_used |
There was a problem hiding this comment.
Review of the Aviti SELECT clause. Apologies for all the questions, it's possible some of these have been discussed before and I'm not aware. Happy to talk through if easier.
- Lane - retrieved from a different field in Illumina vs Aviti. If you retrieve both fields for Aviti, the values do not match! Not sure why, or which field is 'correct' for this use case.
- reagent_kit_barcode and flowcell SerialNumber don’t seem like the same thing, but are put into the same column
- (minor) custom_primer_kit_used retrieved from LIMS data rather than instrument data - assume because not available in instrument data for Aviti?
- The following pairings of 'equivalent fields' seem legitimate, but the set of data values do not match between Illumina and Aviti - should be either transformed to be the same, or the difference documented and agreed with customer:
- kit_type and ThroughputSelection (also included as a separate field at the end)
- cycle_number and KitConfiguration (also included as a separate field at the end)
- Qc outcome from different sources
- read1, read2 - pls check if the values match
- Date completed - run completion date from a timestamp of RunUploaded.json file - as opposed to qc_complete date in Illumina query - is this OK?
- Hardcoding total to 1 - justification?
| f.custom_primer_kit_used, | ||
| f.quant_method_used, | ||
| r.run_parameters | ||
| ORDER BY st.name, f.cost_code, f.lane; |
There was a problem hiding this comment.
Copilot flagged this -
Just checking you're aware the ORDER BY applies to the whole data set, not just the second half after the UNION.
You're using table aliases that only make sense in the context of the Aviti query. If you want it to apply to the whole data set, the aliases to use are study_name, project_cost_code, and lane_position.
|
Minor formatting / readability comments:
|
2 participants
Closes #Y25-613
Summary
Adds
billing_report_view_v3which extends the existing Illumina billing query to include Aviti sequencing run billing data.Changes
billing_report_view_v3combining Illumina and Aviti billing data via UNION ALLeseq_run->eseq_run_lane_metrics->eseq_product_metrics->eseq_flowcellplus sharedstudyandsampletablesthroughput_selection- extracted fromeseq_run.run_parametersJSONkit_configuration- extracted fromeseq_run.run_parametersJSONelement_customer_primer_kit- fromeseq_flowcell.custom_primer_kit_usedelement_quant_method_used- fromeseq_flowcell.quant_method_usedbilling_report_view_v2included unchanged with NULL placeholders for the 4 new Aviti-only columns.